fix(recap): surface real auth/network errors on first run#1168
Merged
Conversation
The auth pre-check in runRecap collapsed three distinct failure modes (missing token, keyring read error, insecure base URL) into a single "Sign in with `entire login`" message, with the real error swallowed by NewSilentError. This made flags appear broken on first run because the function exited before any line that consumed --week, --agent, etc. Replace the gating call with a newRecapClient helper that builds the client even with an empty token, so FetchMeRecap always runs and surfaces the actual cause via the existing recapLoadErrorMessage mapping (401 → "Run `entire login` to re-authenticate.", 4xx/5xx → specific guidance). The insecure-URL check stays but only when a token exists, with a friendlier message that names the offending env var and the fix. Also split DNS NXDOMAIN out of the generic network-error path so a misconfigured ENTIRE_API_BASE_URL says "Could not resolve API host..." instead of blaming the user's internet connection. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> Entire-Checkpoint: d534f23017c5
Contributor
There was a problem hiding this comment.
Pull request overview
Updates entire recap startup/auth behavior so first-run failures surface the real underlying API/auth/network error (instead of being collapsed into a generic “sign in” message), and improves network error messaging by distinguishing DNS NXDOMAIN from general connectivity issues.
Changes:
- Replace the early auth “gate” with a
newRecapClienthelper that allows recap fetch to run (even with an empty token) and lets existing error mapping handle 401/4xx/5xx cases. - Improve insecure base URL handling to produce a friendlier, more actionable message when auth would be sent over HTTP.
- Add explicit NXDOMAIN detection to show a “Could not resolve API host…” message, plus a unit test covering it.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/entire/cli/recap.go | Builds recap API client without gating on missing token; special-cases insecure HTTP auth error messaging. |
| cmd/entire/cli/recap_errors.go | Adds NXDOMAIN detection + tailored DNS error message ahead of generic network error messaging. |
| cmd/entire/cli/recap_test.go | Adds TestRecapLoadErrorMessage_DNSNotFound to validate the new NXDOMAIN message behavior. |
Address Copilot review on #1168: newRecapClient was discarding the error from auth.LookupCurrentToken, which silently turned a keyring read failure (locked, permission denied, etc.) into a "no token" state. That re-introduced the same collapsed-error UX the PR set out to fix — the user would be told to run `entire login`, which can't help when the keyring itself is the problem. Wrap the underlying error in a *keyringReadError (preserving the cause via errors.As) and route it in runRecap to a targeted message that states the keyring is the problem and that re-login is unlikely to help. Truly-missing tokens (keyring.ErrNotFound resolves to "", nil upstream) still flow through to FetchMeRecap and the existing 401 → "Run `entire login` to re-authenticate." mapping. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> Entire-Checkpoint: 60d4f6868ab0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://entire.io/gh/entireio/cli/trails/342
Summary
runRecapcollapsed three distinct failure modes (missing token, keyring read error, insecure base URL) into oneSign in with \entire login`message, with the real error swallowed byNewSilentError. Flags appeared broken on first run because the function exited before any line that consumed--week,--agent`, etc.newRecapClienthelper that builds the client even with an empty token, soFetchMeRecapalways runs and surfaces the actual cause via the existingrecapLoadErrorMessagemapping (401 → "Runentire loginto re-authenticate.", 4xx/5xx → specific guidance).ENTIRE_API_BASE_URLsays "Could not resolve API host…" instead of blaming the user's internet connection.Test plan
mise run checkpasses (fmt + lint + unit + integration + canary E2E, ~144s)entire recap --static --dayrenders datahttp://URL without--insecure-http-auth→ friendly env-var message (not "sign in")entire loginto re-authenticate." for--day/--week/--month/--90TestRecapLoadErrorMessage_DNSNotFoundcovers NXDOMAIN🤖 Generated with Claude Code
Note
Medium Risk
Adjusts
entire recapauthentication/client initialization and error messaging, which can affect how secure/insecure API base URLs are handled and what users see on failures. While scoped to the recap command, regressions could change login prompting or mask real connectivity issues.Overview
entire recapno longer preemptively fails when the auth token is missing or unreadable; it now builds an API client vianewRecapClient(even with an empty token) so the recap fetch runs and surfaces the actual 401/auth failure through existing error mapping.Insecure
http://ENTIRE_API_BASE_URLhandling is tightened to only block when a token exists (unless--insecure-http-authis set) and now prints a clearer, env-var-specific remediation message.Recap error messaging now distinguishes DNS NXDOMAIN from generic network failures, returning a targeted “Could not resolve API host…” hint, with a new unit test covering the NXDOMAIN case.
Reviewed by Cursor Bugbot for commit 98b4106. Configure here.